Merged
Conversation
dbrakenhoff
reviewed
Sep 19, 2024
| AHN3: 'AHN3_05m_DSM', 'AHN3_05m_DTM', 'AHN3_5m_DSM' or 'AHN3_5m_DTM' | ||
| AHN4: 'AHN4_DTM_05m', 'AHN4_DTM_5m', 'AHN4_DSM_05m' or 'AHN4_DSM_5m' | ||
| The default is 'AHN4_DTM_5m'. | ||
| AHN1: 'AHN1_5M' |
Collaborator
There was a problem hiding this comment.
Add explanation of M and R :)
dbrakenhoff
reviewed
Sep 19, 2024
| if not r.ok: | ||
| raise (HTTPError(f"Request not successful: {r.url}")) | ||
| gdf = gpd.GeoDataFrame.from_features(r.json()["result"]["features"], crs=4326) | ||
| gdf = gdf.to_crs(crs) |
Collaborator
There was a problem hiding this comment.
The transform here is happening using EPSG 28992, not our custom definition. Are we sure things are ending up in the right spot in NL?
dbrakenhoff
reviewed
Sep 19, 2024
nlmod/read/ahn.py
Outdated
| extent | ||
| identifier : TYPE, optional | ||
| identifier : str, optional | ||
| Possible values are 'AHN2_05M_I', 'AHN2_05M_N', 'AHN2_05M_R' and |
Collaborator
There was a problem hiding this comment.
explain differences between suffixes?
dbrakenhoff
approved these changes
Sep 19, 2024
Collaborator
dbrakenhoff
left a comment
dbrakenhoff
left a comment
There was a problem hiding this comment.
One question about the transform from EPSG4326 to EPSG28992, whether that is currently correct, as compared to our background map methods?
For the rest some guidance on which identifier corresponds to which AHN product would be nice.
Otherwise looks good, so approving this PR.
This was referenced Sep 19, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes issues #367 and #369, by adding support to AHN 5, and implementing a different way to retrieve the ahn-data.
This PR changes the way we retrieve the AHN data for all AHN-versions, to use the Ellipsis Drive (described in https://www.ahn.nl/dataroom).
get_ahn1,get_ahn2,get_ahn3andget_ahn4have been renamed toget_ahn1_legacy,get_ahn2_legacy,get_ahn3_legacyandget_ahn4_legacy, mainly so the user can test if the same output is returned by the new methods: new versions ofget_ahn1,get_ahn2,get_ahn3,get_ahn4andget_ahn5have been implemented that use the files from Ellipsis Drive. These methods will always return a xarray DataArray, so the use of theas_data_array-argument has been deprecated. Because the merging of the different tiles now takes place at the rioxarray-level (and not at the rasterio-level), the bug described in #369 is solved.The identifier-argument on the ellipsis-drive (which determines the resolution of the downloaded data and if a DTM or DSM is downloaded) is a bit different from the ones we used to use. Some logic has been implemented, so the new methods should also work with the old identifier-names.